Skip to content

os: fix cpus invalid return type jsdoc syntax#43006

Merged
aduh95 merged 2 commits intonodejs:masterfrom
himself65:20220508-fix-jsdoc
May 10, 2022
Merged

os: fix cpus invalid return type jsdoc syntax#43006
aduh95 merged 2 commits intonodejs:masterfrom
himself65:20220508-fix-jsdoc

Conversation

@himself65
Copy link
Member

@himself65 himself65 commented May 8, 2022

figure 1

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. os Issues and PRs related to the os subsystem. labels May 8, 2022
@himself65 himself65 added the fast-track PRs that do not need to wait for 48 hours to land. label May 8, 2022
@github-actions
Copy link
Contributor

github-actions bot commented May 8, 2022

Fast-track has been requested by @himself65. Please 👍 to approve.

RaisinTen
RaisinTen reviewed May 8, 2022
View reviewed changes
Copy link
Member

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This syntax is used in a couple of other places too, like

node/lib/os.js

Lines 331 to 336 in 58d8d69

* @returns {{
* uid: number
* gid: number
* username: string
* homedir: string
* shell: string | null
. We should probably have a linter rule to enforce this in

node/.eslintrc.js

Lines 304 to 314 in 58d8d69

// JSDoc recommended rules that we disable
'jsdoc/require-jsdoc': 'off',
'jsdoc/require-param-description': 'off',
'jsdoc/newline-after-description': 'off',
'jsdoc/require-returns-description': 'off',
'jsdoc/valid-types': 'off',
'jsdoc/no-undefined-types': 'off',
'jsdoc/require-param': 'off',
'jsdoc/check-tag-names': 'off',
'jsdoc/require-returns': 'off',
'jsdoc/require-property-description': 'off',
, otherwise future changes might unintentionally undo your change.

@himself65 himself65 force-pushed the 20220508-fix-jsdoc branch from 08110f4 to 075b04f Compare May 8, 2022 16:30
@himself65
Copy link
Member Author

himself65 commented May 8, 2022

This syntax is used in a couple of other places too, like

node/lib/os.js

Lines 331 to 336 in 58d8d69

* @returns {{
* uid: number
* gid: number
* username: string
* homedir: string
* shell: string | null

. We should probably have a linter rule to enforce this in

node/.eslintrc.js

Lines 304 to 314 in 58d8d69

// JSDoc recommended rules that we disable
'jsdoc/require-jsdoc': 'off',
'jsdoc/require-param-description': 'off',
'jsdoc/newline-after-description': 'off',
'jsdoc/require-returns-description': 'off',
'jsdoc/valid-types': 'off',
'jsdoc/no-undefined-types': 'off',
'jsdoc/require-param': 'off',
'jsdoc/check-tag-names': 'off',
'jsdoc/require-returns': 'off',
'jsdoc/require-property-description': 'off',

, otherwise future changes might unintentionally undo your change.

fixed

@himself65
Copy link
Member Author

himself65 commented May 8, 2022

I ran the make lint-js-fix, and there were many codes breaks the rule, for example.

So I don't think we should enable the jsdoc type check for now

For example:

/Users/himself65/Code/node/lib/util.js
   85:1  warning  Syntax error in type: arg is boolean                                                                                                                                    jsdoc/valid-types
   94:1  warning  Syntax error in type: arg is null                                                                                                                                       jsdoc/valid-types
  103:1  warning  Syntax error in type: arg is (null | undefined)                                                                                                                         jsdoc/valid-types
  112:1  warning  Syntax error in type: arg is number                                                                                                                                     jsdoc/valid-types
  120:1  warning  Syntax error in type: arg is string                                                                                                                                     jsdoc/valid-types
  129:1  warning  Syntax error in type: arg is symbol                                                                                                                                     jsdoc/valid-types
  138:1  warning  Syntax error in type: arg is undefined                                                                                                                                  jsdoc/valid-types
  147:1  warning  Syntax error in type: a is NonNullable<object>                                                                                                                          jsdoc/valid-types
  156:1  warning  Syntax error in type: arg is Error                                                                                                                                      jsdoc/valid-types
  165:1  warning  Syntax error in type: arg is Function                                                                                                                                   jsdoc/valid-types
  174:1  warning  Syntax error in type: arg is (boolean | null | number | string | symbol | undefined)                                                                                    jsdoc/valid-types
  209:1  warning  Syntax error in type: (...args: any[]) => void                                                                                                                          jsdoc/valid-types
  259:1  warning  Syntax error in type: S extends null ? T : (T & S)                                                                                                                      jsdoc/valid-types
  285:1  warning  @template should not have a bracketed type in "jsdoc" mode                                                                                                              jsdoc/valid-types
  287:1  warning  Syntax error in type: T extends (...args: infer TArgs) => Promise<infer TReturn> ?
  ((...params: [...TArgs, ((err: Error, ret: TReturn) => any)]) => void) :
  never
  jsdoc/valid-types

@himself65
Copy link
Member Author

himself65 commented May 8, 2022

I check the eslint-jsdoc rules, and if we wanna use typescript type decelation.

we should set

settings: {
  jsdoc: {
    mode: 'typescript'
  }
},

@RaisinTen
Copy link
Member

RaisinTen commented May 9, 2022

I ran the make lint-js-fix, and there were many codes breaks the rule, for example.

So I don't think we should enable the jsdoc type check for now

If the problem is about adding one more revision to the git blame, I think we could still do the change and add the commit to the .git-blame-ignore-revs file. For reference, Electron does that in https://github.com/electron/electron/blob/main/.git-blame-ignore-revs.

cc @targos since you mentioned the .git-blame-ignore-revs file in #42752 (comment).

aduh95
aduh95 reviewed May 9, 2022
View reviewed changes
@aduh95 aduh95 added the typings label May 9, 2022
targos
targos reviewed May 9, 2022
View reviewed changes
lib/os.js
Comment on lines +122 to +123
* model: string,
* speed: number,
Copy link
Member

@targos targos May 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a personal preference of using ; in type definitions, but I'm fine with , if we already use it elsewhere.

@himself65 himself65 mentioned this pull request May 9, 2022
Closed
@himself65
Copy link
Member Author

himself65 commented May 9, 2022 •
edited
Loading

I've opened a new issue that discusses whether enable jsdoc/valid-types

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 9, 2022
@himself65 himself65 added the commit-queue Add this label to land a pull request using GitHub Actions. label May 10, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels May 10, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 10, 2022

Commit Queue failed
- Loading data for nodejs/node/pull/43006
✔  Done loading data for nodejs/node/pull/43006
----------------------------------- PR info ------------------------------------
Title      os: fix `cpus` invalid return type jsdoc syntax (#43006)
Author     Himself65  (@Himself65)
Branch     Himself65:20220508-fix-jsdoc -> nodejs:master
Labels     os, fast-track, author ready, needs-ci, typings
Commits    2
 - os: fix `cpus` invalid return type jsdoc syntax
 - fixup!os: fix `cpus` invalid return type jsdoc syntax
Committers 1
 - Himself65 
PR-URL: https://github.com/nodejs/node/pull/43006
Reviewed-By: Antoine du Hamel 
Reviewed-By: Darshan Sen 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/43006
Reviewed-By: Antoine du Hamel 
Reviewed-By: Darshan Sen 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 08 May 2022 05:08:56 GMT
   ✔  Approvals: 2
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/43006#pullrequestreview-966523299
   ✔  - Darshan Sen (@RaisinTen) (TSC): https://github.com/nodejs/node/pull/43006#pullrequestreview-967159779
   ℹ  This PR is being fast-tracked
   ✖  The fast-track request requires at least two collaborators' approvals (👍).
   ✖  Last GitHub CI failed
   ✖  No Jenkins CI runs detected
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/2303698988

@aduh95 aduh95 removed fast-track PRs that do not need to wait for 48 hours to land. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels May 10, 2022
@aduh95 aduh95 merged commit d62e412 into nodejs:master May 10, 2022
@aduh95
Copy link
Contributor

aduh95 commented May 10, 2022

Landed in d62e412

@himself65 himself65 deleted the 20220508-fix-jsdoc branch May 11, 2022 00:00
BethGriggs pushed a commit that referenced this pull request May 16, 2022
@himself65 @BethGriggs
typings: fix os.cpus invalid return type
e9e1f1e 
PR-URL: #43006
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request May 16, 2022
Merged
juanarbol pushed a commit that referenced this pull request May 31, 2022
@himself65 @juanarbol
typings: fix os.cpus invalid return type
6de289b 
PR-URL: #43006
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@juanarbol juanarbol mentioned this pull request May 31, 2022
Closed
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
@himself65 @danielleadams
typings: fix os.cpus invalid return type
9e811d6 
PR-URL: #43006
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos pushed a commit that referenced this pull request Jul 12, 2022
@himself65 @targos
typings: fix os.cpus invalid return type
da5f364 
PR-URL: #43006
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
@himself65 @targos
typings: fix os.cpus invalid return type
54e7d0d 
PR-URL: #43006
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@targos targos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@himself65 @guangwong
typings: fix os.cpus invalid return type
d703373 
PR-URL: nodejs/node#43006
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@targos targos targos left review comments

@aduh95 aduh95 aduh95 approved these changes

@RaisinTen RaisinTen RaisinTen approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. os Issues and PRs related to the os subsystem. typings

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@himself65 @RaisinTen @nodejs-github-bot @aduh95 @targos